-
Notifications
You must be signed in to change notification settings - Fork 1.8k
let_unit_with_type_underscore
: make early-pass
#15458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @samueltardieu. Use |
43b866d
to
b506c33
Compare
I won't be able to properly review this PR in the next two weeks, I'll reroll the dice. |
b506c33
to
b40274c
Compare
span_lint_and_then( | ||
cx, | ||
LET_WITH_TYPE_UNDERSCORE, | ||
local.span, | ||
"variable declared with type underscore", | ||
|diag| { | ||
diag.span_suggestion_verbose( | ||
span_to_remove, | ||
ty.span.with_lo(local.pat.span.hi()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work the local and type are in different contexts. The old one is the correct way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit unfortunate, because removing span_to_remove
was kind of the whole purpose of this PR.. I'm the one who added it in #15386
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work [if] the local and type are in different contexts
Actually, we don't even lint if that's the case:
&& local.span.eq_ctxt(ty.span) |
So I think this is fine after all?
Or do you think we can instead remove that check now? I tried doing that, then I needed to modfify span_to_remove
to be ty.span.with_lo(local.pat.span.source_callsite().hi())
, but then all of the following test cases passed at least:
macro_rules! foo {
() => {
(a)
};
};
macro_rules! bar {
() => {
_
};
};
let (a): bar!() = 0;
//~^ let_with_type_underscore
let foo!(): _ = 0;
//~^ let_with_type_underscore
let foo!(): bar!() = 0;
//~^ let_with_type_underscore
gives:
error: variable declared with type underscore
--> tests/ui/let_with_type_underscore.rs:71:5
|
LL | let (a): bar!() = 0;
| ^^^^^^^^^^^^^^^^^^^^
|
help: remove the explicit type `_` declaration
|
LL - let (a): bar!() = 0;
LL + let (a) = 0;
|
error: variable declared with type underscore
--> tests/ui/let_with_type_underscore.rs:73:5
|
LL | let foo!(): _ = 0;
| ^^^^^^^^^^^^^^^^^^
|
help: remove the explicit type `_` declaration
|
LL - let foo!(): _ = 0;
LL + let foo!() = 0;
|
error: variable declared with type underscore
--> tests/ui/let_with_type_underscore.rs:75:5
|
LL | let foo!(): bar!() = 0;
| ^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove the explicit type `_` declaration
|
LL - let foo!(): bar!() = 0;
LL + let foo!() = 0;
|
But:
- the error message mentions
_
instead ofbar!()
- this could be seen as a feature addition
So I'd say leaving the eq_ctxt
in and simplifying span_to_remove
is good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the pattern and type. e.g. let $name: _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe that's what the check should've been?
b40274c
to
c90e863
Compare
this should address all points except #15458 (comment) |
2d8b1dd
to
2d3dcf5
Compare
to the existing function as well
2d3dcf5
to
2459ad6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
This kind of supersedes #15386 -- by making the lint early-pass, we get access to
TyKind::Paren
s that surround the type ascription. And with that, we can return to the simpler calculation ofspan_to_remove
.The biggest hurdle was
is_from_proc_macro
-- calling that function required me toimpl WithSearchPat for rustc_ast::Ty
, i.e.ast_ty_search_pat
, which I based onty_search_pat
. Since that's a larger change, I could extract it into a PR of its own.changelog: none